Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "vm: add importModuleDynamically option to compileFunction" #33364

Closed
wants to merge 1 commit into from
Closed

Revert "vm: add importModuleDynamically option to compileFunction" #33364

wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

This reverts commit 74c393d.

Fixes: #33166
See: #32985
See: #31860

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. vm Issues and PRs related to the vm subsystem. labels May 12, 2020
@mcollina
Copy link
Member Author

@SimenB this should likely fix Babel and Jest.

@mcollina mcollina mentioned this pull request May 12, 2020
Closed
@SimenB
Copy link
Member

SimenB commented May 12, 2020

I hope it can re-land later as I wanna use the code this reverts. 😀 But it does seem prudent to revert for now.
I wonder if the problem points to some other deep-lying issue that this change just exposed, though. If building using a different compiler changes things it seems likely this JS change is just a symptom and not the cause.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31286/

targos
targos reviewed May 12, 2020
View reviewed changes
doc/api/vm.md Outdated Show resolved Hide resolved
benjamingr
benjamingr approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp LGTM due to revert + severity.

(Plus a non-rubber-stamp-lgtm on the delta like the md changes)

@mcollina
Copy link
Member Author

mcollina commented May 12, 2020 •
edited
Loading

I wonder if the problem points to some other deep-lying issue that this change just exposed, though. If building using a different compiler changes things it seems likely this JS change is just a symptom and not the cause.

I literally have no clue on how to debug that. I know there is something in vs2019 that is affecting stdout, but it's really very hard to track. cc @ronag #32487

@targos
Copy link
Member

targos commented May 12, 2020

@mcollina is #33166 also reproducible on nightlies from master? If not, I think we should actually only revert on v14.x.

@mcollina
Copy link
Member Author

It fails also on the nightlies

@devsnek
Copy link
Member

devsnek commented May 12, 2020

I still don't get why we consider this commit the issue and not vs2019

@mcollina
Copy link
Member Author

I still don't get why we consider this commit the issue and not vs2019

@devsnek because this commit triggers the problem. Node v14 is largely broken on one of the supported Operating Systems, and it is important that we ship a fix sooner rather than later. However I have got no clue on how to approach the vs2019 issue and it might take an unbounded amount of time to fix it. We currently have no strategy around this problem.

I'm currently pinging some connections in Microsoft to see if they can help, but I fear it might take a long time to get to a conclusion as this problem is very ephemeral.

@devsnek
Copy link
Member

devsnek commented May 12, 2020

@mcollina I mean why are we saying "build without this commit" instead of "build without vs2019". This issue could easily manifest itself in other ways (I think jasnell mentioned stream timing bugs?) so it seems like we should use a better compiler.

@mcollina
Copy link
Member Author

I do not know how long would it take for the @nodejs/build to change the compiler we are using to build our Node.js releases. I suspect it's a significant amount of effort. I do not think we can wait fixing this bug on v14.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 12, 2020
@addaleax
Copy link
Member

I’m a bit worried here too … I get that this is a severe problem, but without understanding why this commit causes it – it almost certainly only exposes a pre-existing bug – there’s a decent chance that we’ll do some other thing that re-exposes it in the future without knowing.

@jasnell
Copy link
Member

jasnell commented May 12, 2020

Yep. I consider this revert to be temporary to fix the acute breakage but the revert itself is not the solution.

mhdawson
mhdawson approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cjihrig cjihrig mentioned this pull request May 14, 2020
Closed
2 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31344/

@mcollina
Copy link
Member Author

Landed in 2d5d773

@mcollina mcollina closed this May 15, 2020
mcollina added a commit that referenced this pull request May 15, 2020
@mcollina
Revert "vm: add importModuleDynamically option to compileFunction"
2d5d773 
This reverts commit 74c393d.

Fixes: #33166

PR-URL: #33364
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request May 16, 2020
@mcollina @codebytere
Revert "vm: add importModuleDynamically option to compileFunction"
c2cf978 
This reverts commit 74c393d.

Fixes: #33166

PR-URL: #33364
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request May 18, 2020
Merged
devsnek added a commit to devsnek/node that referenced this pull request Sep 29, 2020
@devsnek
Revert "Revert "vm: add importModuleDynamically option to compileFunc…
12aff38 
…tion""

This reverts commit 2d5d773.

See: nodejs#32985
See: nodejs#33364
See: nodejs#33166
Fixes: nodejs#31860
@devsnek devsnek mentioned this pull request Sep 30, 2020
Merged
4 tasks
devsnek added a commit to devsnek/node that referenced this pull request Feb 5, 2021
@devsnek
Revert "Revert "vm: add importModuleDynamically option to compileFunc…
056a671 
…tion""

This reverts commit 2d5d773.

See: nodejs#32985
See: nodejs#33364
See: nodejs#33166
Fixes: nodejs#31860
devsnek added a commit to devsnek/node that referenced this pull request Feb 5, 2021
@devsnek
Revert "Revert "vm: add importModuleDynamically option to compileFunc…
28f5ec8 
…tion""

This reverts commit 2d5d773.

See: nodejs#32985
See: nodejs#33364
See: nodejs#33166
Fixes: nodejs#31860
devsnek added a commit that referenced this pull request Feb 5, 2021
@devsnek
vm: add importModuleDynamically option to compileFunction
bf2f2b7 
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@devsnek @danielleadams
vm: add importModuleDynamically option to compileFunction
66a14d3 
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@devsnek devsnek Awaiting requested review from devsnek

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. tsc-agenda Issues and PRs to discuss during the meetings of the TSC. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Regression in 14.1.0 - Windows] stdout is sometimes empty